#75 program BPF maps before attach to eliminate startup-race drops#76
Merged
Conversation
Signed-off-by: Matthew DeVenny <matt@codecargo.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a startup “readiness” boundary so that blocked events occurring before CargoWall is fully ready (the attach-before-program startup race window) are suppressed from audit logging and notifications, while still being visible in the process logs. It also improves GitHub Actions log output by adding a millisecond timestamp prefix.
Changes:
- Add a
Readinesshelper (monotonic timestamp boundary) and gate blocked-event reporting (audit + notifications) on it. - Wire readiness through startup and event processing so pre-ready blocks are excluded.
- Prefix GitHub Actions log output with a timestamp (ms precision) and add coverage for the handler and readiness gate.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/events/events.go | Adds Readiness boundary and suppresses pre-ready blocked events from audit/notifications. |
| pkg/events/events_test.go | Updates processEvent call sites and adds readiness-gate test coverage. |
| cmd/start.go | Creates readiness, passes it to event processing, and marks ready just before publishing readiness. |
| cmd/github_actions_handler.go | Adds millisecond timestamp prefix to GitHub Actions log formatting. |
| cmd/github_actions_handler_test.go | Adds tests validating the timestamp placement and prefix semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The "a zero event timestamp never suppresses" guarantee only holds after MarkReady. Before readiness (readyMono == 0) the whole window is pre-ready, so a zero/garbage-stamped event is suppressed too. Fix the doc comment to state the precedence accurately and add a (readyMono=0, eventMono=0) case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew DeVenny <matt@codecargo.com>
Replace the reporting-layer suppression with a root-cause fix. Attach the TC egress program only AFTER the default action, static allowlist, auto-allow infrastructure rules, tracked hostnames, and existing connections are programmed into the maps (the maps exist independently of the link). The firewall is therefore correct from its first enforced packet, so the attach-before-program race that produced spurious "Connection blocked" drops (e.g. the Azure wireserver on :32526) no longer happens — there is nothing to suppress, and real startup-window blocks stay honest, reported policy decisions. Removes the Readiness type/gate/MarkReady and its processEvent threading (now unnecessary) and the readiness tests. Keeps the per-line process-log timestamps (the redundant per-event timestamp= field stays removed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew DeVenny <matt@codecargo.com>
matthewdevenny
changed the title
Drain the pipe's read end in a goroutine (so r is always consumed and closed), and restore os.Stderr + close the write end via defer even if fn panics. The concurrent drain also removes the >64KB pipe-buffer deadlock risk. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew DeVenny <matt@codecargo.com>
Address the review nits on the process-log timestamp: - Render in UTC so it aligns with GitHub's own gutter timestamps and is unambiguous across runner timezones. - Only plain (Info) lines get the leading timestamp; ::error::/::warning::/ ::debug:: keep a bare workflow command with no injected timestamp, so GitHub still de-duplicates identical annotations instead of treating each as unique. - Timestamp every physical line of a multi-line plain record, and escape embedded newlines (%0A, with % and CR) in workflow-command messages so a multi-line message stays a single annotation rather than leaking un-prefixed continuation lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew DeVenny <matt@codecargo.com>
The multi-line plain-line path emitted one Fprintf per physical line, so a concurrent log from another goroutine could interleave between a record's lines and scramble multi-line output. Buffer the whole formatted record into a strings.Builder and write it once, restoring the original one-write-per-record behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew DeVenny <matt@codecargo.com>
matthewdevenny
marked this pull request as ready for review
June 29, 2026 18:22
A plain Info record whose assembled body ended in "\n" produced a stray timestamp-only line, because strings.SplitSeq yields a trailing empty element. Trim a single trailing newline before splitting, and add a regression test for an attribute value ending in "\n". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew DeVenny <matt@codecargo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eliminates the startup-race "Connection blocked" false positives at the source by programming the BPF maps before the TC egress program is attached, and adds per-line timestamps to the GitHub Actions process log.
Closes #75.
Why
During startup the TC egress program was attached (
AttachEgress) before the auto-added infrastructure allow rules were written into the BPF maps. The program was therefore live in default-deny with an empty allowlist, so a connection in that window was dropped once and self-healed on TCP retransmit — but still got recorded as a false-positive policy block (e.g. the Azure wireserver168.63.129.16:32526/ WALinuxAgent).(This was first mistaken for #71 — the all-ports-vs-port-specific shared-IP bug. It wasn't: that IP has only port-specific CIDR rules, and the same connection was allowed moments later. It's purely an attach-before-program ordering race.)
What changed
Root-cause fix (
cmd/start.go): attach the TC egress program only after the maps are fully programmed — default action, static allowlist (UpdateAllowlistTC), auto-allow infrastructure (applyAutoAllowHelpers), tracked hostnames (ApplyRulesToTrackedHostnames), and existing connections (gateExistingConnections). The BPF maps exist independently of the link, so this is a pure reordering. The firewall is now correct from its first enforced packet, so no spurious startup drop is ever generated. It also tightens failure semantics: a programming error now returns before attaching, instead of leaving a deny-all program attached. Dynamic DNS-resolved hosts are still handled in-band by the existing late-add path inprocessEvent.Process-log timestamps (
cmd/github_actions_handler.go): plain (Info) log lines now carry a leading UTC millisecond timestamp — every physical line, so a multi-line attribute value (e.g. a wrapped error) stays timestamped. Workflow-command levels (::error::/::warning::/::debug::) keep a bare command with no injected timestamp, so GitHub still de-duplicates identical annotations (a unique per-line timestamp would defeat dedup and inflate the per-run annotation count); their messages have newlines escaped (%0A) so a multi-line message stays one annotation. The redundant hand-rolledtimestamp=field on block lines is removed (the JSON handler already emitstime).Approach note
An earlier iteration of this PR suppressed pre-ready blocks at the reporting layer (a
Readinessmonotonic-timestamp gate inprocessEvent). Per review (the race is better prevented than masked), that machinery has been removed in favor of the reordering above — which also avoids the gate's edge cases (it would have suppressed real policy blocks across the multi-second Docker restart, under-reported in audit mode, and missed one-shot UDP/ICMP). Real startup-window blocks are now honest, reported policy decisions; there simply are no spurious ones to hide.Net effect
The wireserver-style startup drop no longer occurs at all — the infra rule is in the map before enforcement begins. Genuine policy blocks during startup remain fully reported (audit log, SaaS, summary, notifications) with a timestamped process-log line.
Files
cmd/start.go— moveAttachEgressto after map programming; start the event reader before enforcement.cmd/github_actions_handler.go+cmd/github_actions_handler_test.go— per-line millisecond timestamp.pkg/events/events.go— drop the redundant per-eventtimestamp=field.Testing
GOOS=linuxgo build/go vet/staticcheck/gofumpt -lall clean. The reorder is exercised end-to-end on CI (eBPF build tags are linux-only); the firewall-setup unit tests (gateExistingConnections,applyAutoAllowHelpers) are unaffected.Follow-ups (separate)
```ansifence).🤖 Generated with Claude Code